Repository navigation
[SYCL] fix e2e atomic ref tests - #23559
Conversation
|
@intel/llvm-gatekeepers |
| @@ -1,5 +1,8 @@ | |||
| // RUN: %if arch-intel_gpu_cri %{ %{build} -DFULL_ATOMIC16_COVERAGE -DFULL_ATOMIC32_COVERAGE -DFULL_ATOMIC64_COVERAGE -fsycl-device-code-split=per_kernel -Xspirv-translator -spirv-ext=+SPV_KHR_bfloat16,+SPV_INTEL_16bit_atomics -o %t.out %} | |||
| // RUN: %if arch-intel_gpu_cri %{ %{run} %t.out %} | |||
| // REQUIRES: arch-intel_gpu_cri | |||
There was a problem hiding this comment.
@dklochkov-emb Is there a particular reason we're limiting all atomic tests to CRI only?
There was a problem hiding this comment.
@againull yes, it is not confirmed that other drivers support atomics.
Moreover, all tests were disabled before.
There was a problem hiding this comment.
@againull in case of the build line
%if arch-intel_gpu_cri %{ %{build}..
fusion CI somehow skipped it but in case of run %if arch-intel_gpu_cri %{ %{run} %t.out %} somehow it tried to run resulting error that binary was not found
There was a problem hiding this comment.
@againull yes, it is not confirmed that other drivers support atomics.
It may apply to the new 16-bit atomics, but not to atomics generally: 32-bit atomics are required by SYCL, and the existing tests were already running and passing on other platforms, including PVC.(https://github.com/intel/llvm/actions/runs/35870937631/job/107222525795).
For the 16-bit tests, we should at least use REQUIRES: aspect-ext_oneapi_atomic16 instead of hardcoding arch-intel_gpu_cri. Ideally, the 16-bit coverage should be in separate test wrappers: combining it with the existing 32-bit tests under a file-wide requirement causes us to lose 32-bit coverage on devices without atomic16 support.
Moreover, all tests were disabled before.
Could you elaborate on this? In this older PVC run (https://github.com/intel/llvm/actions/runs/35870937631/job/107222525795), the atomic tests were executed and passed. Were you referring specifically to the 16-bit cases?
There was a problem hiding this comment.
Ok, I will fix that.
Were you referring specifically to the 16-bit cases?
The fact is that all macro were disabled and they cut almost all tests except..1 data type. In some tests that was float:
https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/AtomicRef/add.h#L540
in some case that was int:
https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/AtomicRef/and.h#L233
I can guess that only reason is that atomic operations with these data types are supported by default and Iadd 32-bit is not supported in evert GPU
I will enable tests by default as it was done earlier.
[PR](#23559) disabled default atomic tests for non-cri targets. This PR restores it by adding local lit config file. If CRI GPU target is detected, it additional macro to enable full coverage of atomic support.
This PR fixes issues of atomic_ref e2e tests. When Fusion CI runs it, build and run lines have different "calculations" of conditions. This makes possible situation when if some_arch will be true for RUN of
%{run}and false for run of%{build}.The reason why I left this in original PR, I did not want to remove unconditional build and run, However, they are useless: they does not build and run any test.